store types as tuple and return ordered dict#30
store types as tuple and return ordered dict#30mikaelarguedas wants to merge 3 commits intoros2:masterfrom
Conversation
mikaelarguedas
added
in review
dirk-thomas
left a comment
dirk-thomas
left a comment
There was a problem hiding this comment.
Beside two minor nitpicks I think it would make more sense to store an actual type object in the new class variable SLOT_TYPES - as suggested in the referenced comment:
Change the data structure from a mutable
dictto an immutabletupleof abstract types
E.g. instead of storing the string rosidl_generator_py/Primitives[<=3] which the caller has to parse I suggested to store rosidl_parser.definition.BoundedSequence(rosidl_parser.definition.NamespacedType(['rosidl_generator_py', 'msg'], 'Primitives'), 3) (verbose to clarify which types I was referring to).
| def get_fields_and_field_types(cls): | ||
| return copy(cls._fields_and_field_types) | ||
| def get_slot_types(cls): | ||
| from collections import OrderedDict |
There was a problem hiding this comment.
Why not import the module at the top of the file?
| ] | ||
|
|
||
| _fields_and_field_types = { | ||
| SLOT_TYPES = (( |
There was a problem hiding this comment.
Why two parenthesis here? One should be sufficient.
Thanks for the clarification, I'll update to use these types instead. |
|
@dirk-thomas Looking a bit more into this, it looks like this work will be conflicting / duplicating effort with #24. Would it be better to wait for #24 to land and build based on the state off master at that point? If not, there is an initial working version at mikaelarguedas@0a88958 that I can push forward. A few questions remain:
Based on this the consumer could instantiate messages of that type and use them (use case from ros2/ros2cli#197) |
That will certainly be the case.
That would be great (to avoid the need for rebasing the patch). I hope that the referenced PR will get merged in the near future.
I am not sure I understand what you mean with "parse these types"? Since the API would return types (rather than strings) there shouldn't be a need to parse anything.
I don't think that distinction is relevant. At least in all the message generators patched for the IDL work I don't recall anywhere where a condition would say "isinstance(t, BaseType) or isinstance(t, BaseString)".
A function to import the module if the type is a |
|
Superseeded by #33 |
2 participants
This is a follow-up of #19
Required to fix ros2/ros2cli#59
@dirk-thomas the returned keys right now are the exact names of the slots (with leading underscore), is it what you expected in your review of #19 (comment) or should the leading underscore be stripped ?